Repository navigation
fix(agent-isolation): scope touch dismissals to each operation - #1894
Conversation
Share real signer identity across wrapper and hook watchers. Clear only an empty completed-authentication dismissal while holding the window lease. Keep conservative fallback and owner cleanup, and cover both regressions. The existing software-key signing test now takes the shared process lock; the setup fingerprint is regenerated by its required hook. Fixes apache#1834 Generated-by: Codex (desktop 26.1013.40934) Generated-by: Claude Desktop 4.29.0 (Opus 5.5 review)
Generated-by: Codex (desktop 26.1016.61520) Generated-by: Claude Desktop 4.29.0 (Opus 5.5 review)
There was a problem hiding this comment.
Thanks @leninworld for the solid PR. This cleanly resolves the two precision issues identified in #1834.
The implementation is well reasoned and thorough:
-
Shared Signer Identity Across Watchers:
Publishing the child shell PID via an intermediate exec wrapper ensures both wrapped and hook watchers observe the exact same signer PID, eliminating the duplicate window cycle when an agent session and a terminal sign concurrently. -
Conservative Publication Fallback:
Insigner_pids(), falling back to$MAGPIE_GPG_TOUCH_PARENTif$MAGPIE_GPG_TOUCH_SIGNER_FILEis unreadable or fails to publish guarantees that a publication issue never suppresses a touch prompt or aborts the signature. -
Lease-Protected Empty Dismissal Cleanup:
Clearing only empty dismissals under the window lease when socket connections drop back to baseline resolves the second SSH authentication prompt suppression without endangering concurrent nonempty signer dismissals. -
Test Suite & Spec Integrity:
The six new regression tests provide comprehensive coverage across both faults, including abnormal process termination and obstructed context paths. The serialization oftest_git_signs_a_commit_through_the_wrapperwithprocess_namedprevents test flakiness, andFRAMEWORK_FINGERPRINTis correctly synchronized with the modified script.
All checks in CI pass cleanly (1,009 tests passed, 0 failures; prek formatting, Ruff, MyPy, and license checks green).
potiuk
left a comment
There was a problem hiding this comment.
Sharing the real signer PID between the wrapped and hook watchers is clean and well tested; the empty-dismissal cleanup should fire only on the watcher's own blocked-to-idle transition, since as written a freshly armed idle watcher can delete another watcher's dismissal of an SSH authentication still in flight.
GitHub has not yet computed whether this branch merges cleanly with its base.
an empty dismissal is cleared when the watcher observes the authentication end (tools/agent-isolation/gpg-touch-overlay.sh:1018)
An empty ssh-authentication dismissal is cleared when
the watcher observes the authentication end, under the shared window lease;
a nonempty signer dismissal is preserved (#1834).
if [[ -e $DISMISSED_MARKER ]] && _lease_acquire; then
if dismissed="$(cat "$DISMISSED_MARKER" 2>/dev/null)" && [[ -z $dismissed ]]; then
rm -f "$DISMISSED_MARKER" 2>/dev/null
DISMISSED_MARKER is one file per host ($RUNTIME_DIR/dismissed), but baseline is each watcher's own socket count at start. So this branch runs for any watcher that is idle by its own baseline, not only the one that saw the authentication end. A hook watcher armed while an SSH authentication is already in flight counts that connection in its baseline, is idle on its first poll, takes the lease (released by the dismissing watcher after show_overlay) and deletes the empty marker; the watcher still blocked on that authentication then fails dismissal_holds and draws a second window for a touch the operator already dismissed. It fails safe (an extra click), but it brings back the duplicate-window case this PR fixes, and on main an idle watcher never touched the marker.
Clearing only on this watcher's own blocked-to-idle transition would match the spec line: set a flag in the blocked branch, run the lease-guarded removal in the idle branch only when it is set, then reset it. A regression test with watcher B shown and dismissed while a client connection stays open, and watcher A armed during that connection, asserting one showing and a surviving marker, would pin it.
Flagged by both the primary and adversarial reviewers.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
The only conflict is the secure-setup fingerprint, restamped by the hook for the merged tree. Generated-by: Claude Opus 5
… own ending The dismissal marker is one per host, but each watcher's baseline is its own socket count at start. A watcher armed while another's SSH authentication was in flight counted that connection in its baseline, was idle from its first poll, and deleted the empty marker the other watcher's authentication still held, so that watcher drew a second window for a touch already dismissed. The cleanup now runs only after this watcher has seen an operation in flight and then idle, as the spec line says, and is retried while the window lease is busy. A two-watcher regression test arms the second watcher during an open connection and checks that the marker survives and no second window opens; it fails on the previous head. Generated-by: Claude Opus 5
potiuk
left a comment
There was a problem hiding this comment.
Thanks for agreeing on the dismissal cleanup. Since the follow-up you prepared was not pushed yet, I pushed the fix as a fixup so this can land, along with the main merge it needed:
- 9d532ef merges
main; the only conflict was the secure-setup fingerprint, restamped for the merged tree. - 3862f8c (
gpg-touch-overlay.sh): the empty-dismissal cleanup now runs only after this watcher has seen an operation in flight and then idle, as the spec line says, so a watcher armed during another watcher's SSH authentication no longer deletes the dismissal that authentication still holds. The flag stays set while the window lease is busy, so the cleanup is retried on the next idle poll rather than skipped.test_a_watcher_armed_mid_authentication_keeps_its_dismissalarms the second watcher during an open connection and checks that the marker survives with one window; it fails on the previous head.
The whole test_gpg_touch_overlay.py passes on macOS too, apart from the Linux-only window skips. Please pull before pushing again, and drop your local version of the same change if it is still around.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Magpie handles maintainer review:
Contributing guide.
This PR fixes issue #1834.
Summary
Type of change
tools/*/withpyproject.toml)Test plan
uv run --frozen prek run --all-files --skip workspace-pytestpasses, including workspace Ruff, formatting, mypy, links and spec checks.Linux ARM64, Python 3.12.12, frozen workspace lock. The skips cover unavailable GUI/Tk, zsh, ambient Node, private-home and trusted-Python scenarios. No physical hardware or macOS GUI validation is claimed. Scoped test-file Ruff reports the same 45 existing diagnostics before and after, with none added.
RFC-AI-0004 compliance
Gen-AI disclosure
Generative AI was used to prepare the code, tests and this description with Codex (desktop version 26.1013.40934), independently reviewed with Claude Desktop 4.29.0 (Opus 5.5). I have personally read and understood the final diff.
Linked issues
Closes #1834.